AsyncTransport._send_http currently raises _TransientError for any HTTP status >= 400 (except the anonymous-retry 4xx early-return). This means permanent client errors like 400/403 will be treated as transient and will repeatedly fall back to disk + retry until expiry, potentially generating lots of on-disk telemetry files without any chance of recovery. Consider classifying 4xx (except possibly 429) as non-transient and dropping them (or logging once), reserving _TransientError for timeouts/connection issues and 5xx.

Telemetry fallback files are written under ~/.kimi (via get_share_dir()) with default mkdir/open permissions, which may be group/world-readable depending on umask. Since these JSONL files include device_id/session_id and environment context, it would be safer to explicitly restrict permissions (e.g., chmod 0o700 on the telemetry dir and 0o600 on failed_*.jsonl files, similar to auth/oauth.py’s _ensure_private_file).

test_anonymous_retry_4xx_drops_events doesn’t currently exercise the “anonymous retry returned 4xx” branch in AsyncTransport._send_http. It patches _send_http itself (bypassing all status-handling logic) and also defines mock_send_http/call_count that are unused. To validate the behavior, mock the aiohttp session.post responses so the first request returns 401 with a token and the anonymous retry returns a 4xx, then assert send() does not call save_to_disk().

The module docstring says the strategy is to “patch kimi_cli.telemetry.track at the call site and assert … rather than running the full UI/soul stack”, but most tests below call track() directly (which will still pass even if the production instrumentation is removed). Either update the docstring to match the intended scope (event schema tests), or change the tests to patch/execute the actual production functions that should emit these events so they truly validate instrumentation correctness.

🟡 Global telemetry state causes event loss and wrong session attribution in multi-session ACP mode

Telemetry uses module-level globals (_sink, _session_id) but KimiCLI.create() is called once per session in multi-session ACP mode (src/kimi_cli/acp/server.py:155-167 and src/kimi_cli/acp/server.py:223-237). Each call to KimiCLI.create() at src/kimi_cli/app.py:296 overwrites _session_id via set_context() and replaces the global _sink via attach_sink() at src/kimi_cli/app.py:310. The previous sink (with buffered events from the earlier session's track("started") and track("startup_perf")) is orphaned — its events are never flushed via HTTP or saved to disk. Additionally, any subsequent track() calls originating from an earlier session's code path will record the wrong session_id (the latest session's ID). Since no periodic flush is started outside the shell UI, and the atexit handler only flushes the latest sink, telemetry events from all but the last-created session are silently lost.

Was this helpful? React with 👍 or 👎 to provide feedback.

🟡 Unguarded telemetry in exception handler can mask original error and skip StopFailure hook

In the _agent_loop exception handler, classify_api_error(e) and track() are called without a try/except guard. If either call raises an unexpected exception, the original API/step error e is replaced by the new exception: the raise at line 826 is never reached, the StopFailure hook at lines 813-825 never fires, and the caller (e.g., run_soul_command) catches the wrong exception type — showing a confusing error instead of the real API error message.

The hook engine at src/kimi_cli/hooks/engine.py:244-254 explicitly wraps its telemetry call in try/except with a comment explaining the safety rationale. The same pattern should be applied here for consistency and correctness.

While classify_api_error only does safe operations on well-typed objects (making failures extremely unlikely), the code path is inside an exception handler where correctness of re-raise semantics is critical.

Was this helpful? React with 👍 or 👎 to provide feedback.

🟡 retry_disk_events does not handle TypeError from _build_payload, causing stuck retry files

send() at src/kimi_cli/telemetry/transport.py:161-173 explicitly catches TypeError from _build_payload() and drops the events, with a comment explaining: "Retrying would hit the same TypeError on every reload, so falling back to disk would just create a permanently stuck file."

However, retry_disk_events() calls _build_payload() at line 297 without the same TypeError guard. If a disk file happens to contain events with non-primitive values in properties/context, the TypeError falls through to the generic except Exception: at line 312, which leaves the file for next startup. This creates exactly the "permanently stuck file" scenario that send() was designed to prevent — the file gets retried on every startup, hitting the same TypeError, until the 7-day expiry cleans it up.

Was this helpful? React with 👍 or 👎 to provide feedback.